Feature/fdb 327 fdb compare #165

stefaniereuter · 2025-08-26T06:36:52Z

Description

Opening this as a draft pull request, while finishing the refactoring on the grib comparison but to start discussing the design and features.

This draft shows the direction in which I would like to continue refactoring.

This tool allows to compare two FDB or two experiments in one FDB. It uses the standard fdb tool mechanisms, minimal-keys, CLI request, but also allows to directly specify mars keys, where entries should be ignored. it uses fdb list to generate a request then sorts the results and compares them, depending on scope, where the default is just a comparison of the mars keys, without the messages itself, possibility of comparing grib-header only and then grib-header plus data section. Default is a key by key comparison,
More explanation and examples can be found here:

https://confluence.ecmwf.int/display/~ecm4053/FDB+Comparison

Contributor Declaration

By opening this pull request, I affirm the following:

All authors agree to the Contributor License Agreement.
The code follows the project's coding standards.
I have performed self-review and added comments where needed.
I have added or updated tests to verify that my changes are effective and functional.
I have run all existing tests and confirmed they pass.

🌈🌦️📖🚧 Documentation 🚧📖🌦️🌈
https://sites.ecmwf.int/docs/dev-section/fdb/pull-requests/PR-165

🌈🌦️📖🚧 Documentation Z3FDB 🚧📖🌦️🌈
https://sites.ecmwf.int/docs/dev-section/z3fdb/pull-requests/PR-165

🌈🌦️📖🚧 Documentation FDB 🚧📖🌦️🌈
https://sites.ecmwf.int/docs/dev-section/fdb/pull-requests/PR-165

stefaniereuter · 2025-08-26T06:37:44Z

This was supposed to be a draft pull request....

pgeier · 2026-01-27T14:51:19Z

Pending tasks:

add license to source and header files
document functions
fix tests
add data check tests

…wo FDB no comparison or testing done yet

…ine options

…nore certain key-value pairs

codecov-commenter · 2026-01-28T15:38:11Z

Codecov Report

❌ Patch coverage is 54.10053% with 347 lines in your changes missing coverage. Please review.
✅ Project coverage is 72.66%. Comparing base (8abf5d2) to head (1b031b4).

Files with missing lines	Patch %	Lines
src/fdb5/tools/compare/grib/CompareBitwise.cc	0.00%	105 Missing ⚠️
src/fdb5/tools/compare/grib/Compare.cc	51.72%	56 Missing ⚠️
src/fdb5/tools/compare/grib/CompareKeys.cc	48.00%	52 Missing ⚠️
src/fdb5/tools/compare/fdb-compare.cc	73.18%	37 Missing ⚠️
src/fdb5/tools/compare/grib/CompareHash.cc	0.00%	32 Missing ⚠️
src/fdb5/tools/compare/common/DataMap.cc	73.68%	20 Missing ⚠️
src/fdb5/tools/compare/common/Util.cc	0.00%	19 Missing ⚠️
src/fdb5/tools/compare/common/Util.h	0.00%	6 Missing ⚠️
src/fdb5/tools/compare/grib/Utils.cc	78.57%	6 Missing ⚠️
src/fdb5/tools/compare/common/ComparisonMap.cc	79.16%	5 Missing ⚠️
... and 2 more

Additional details and impacted files

@@             Coverage Diff             @@
##           develop     #165      +/-   ##
===========================================
- Coverage    73.30%   72.66%   -0.64%     
===========================================
  Files          363      377      +14     
  Lines        21956    22712     +756     
  Branches      2253     2383     +130     
===========================================
+ Hits         16094    16504     +410     
- Misses        5862     6208     +346

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

simondsmart · 2026-01-30T11:05:29Z

tests/fdb/tools/compare/mismatch_mars/CMakeLists.txt

@@ -0,0 +1,7 @@
+ecbuild_configure_file( mismatch_mars.sh.in mismatch_mars.sh @ONLY )
+


I expect to see an ecbuild command getting test data (probably for more than one test), and then the test depending on it.

It isn't great to add meaningful-sized binary files to the git repo...

This approach is following the same approach as the regression tests.
I can avoid duplicating them. Or do you explicitly want them to be uploaded on get.ecmwf.int

simondsmart · 2026-01-30T11:07:31Z

tests/fdb/tools/compare/mismatch_grib/CMakeLists.txt

@@ -0,0 +1,7 @@
+ecbuild_configure_file( mismatch_grib.sh.in mismatch_grib.sh @ONLY )
+


Do the data.grib files differ between the different tests? It looks like you are grib setting them, to set upthe tests on the go. So we probably only need one grib file. You migt even be able to use one which is already referenced in a different test, which would avoid the extra data.

simondsmart · 2026-01-30T11:14:24Z

src/fdb5/tools/compare/fdb-compare.cc

+
+    FDBCompare(int argc, char** argv) : FDBVisitTool(argc, argv, "class,expver") {
+        options_.push_back(new SimpleOption<std::string>("test-config", "Path to a FDB config"));
+        options_.push_back(new SimpleOption<std::string>("reference-config", "Path to a second FDB config"));


Is it important that there is a test and a reference? Is the compare not symmetric? I would have thought that --config-one and --config-two would suffice?

Comparison is symmetric, it's just about naming. Don't have an opinion on that. It's just that the convention is used at various places in the code right now.

simondsmart · 2026-01-30T11:14:52Z

src/fdb5/tools/compare/fdb-compare.cc

+        options_.push_back(new SimpleOption<std::string>("test-config", "Path to a FDB config"));
+        options_.push_back(new SimpleOption<std::string>("reference-config", "Path to a second FDB config"));
+        options_.push_back(new SimpleOption<std::string>(
+            "reference-request", "Mars key request for reference FDB entry (e.g reference experiment)"));


What is a "mars key request" that is not a phrase that makes sense. At the least this needs an example.

simondsmart · 2026-01-30T11:17:09Z

src/fdb5/tools/compare/common/Scope.h

+Method parseMethod(const std::string& s);
+
+
+struct Options {


This is strange being in a file called Scope.

simondsmart · 2026-01-30T11:44:07Z

src/fdb5/tools/compare/grib/Compare.h

+/// @param test Test location of the message
+/// @param opts Additional options
+/// @return Comparison result. As mars is not comparing data, only matched is expected to be set
+Result compareGrib(const DataIndex& ref, const DataIndex& test, const Options& opts);


I must admit that structurally, I would have written this with a comparator class (initialised with Options). And then have the different types of comparison be internal to that class (probably as member functions, but they could be in an anonymous namespace).

Roughly happy with this as is, but stylistically it is not really the same as the rest of the FDB/MARS code in that regard. I would err towards making it so.

I explicitly removed a factory. The comparison calls on mars and grib are explicitly called in order. I don't see a reason to wrap a layer of indirection.

simondsmart · 2026-01-30T11:46:17Z

src/fdb5/tools/compare/fdb-compare.cc

+    }
+    else {
+        fdbtest = FDB(config(args));
+        fdbref  = FDB(config(args));


Note that this re-assignment results in default-initialising (and constructing) two FDBs. Depending on the current environment that may fail (throwing an exception) before you try and construct the FDBs with the supplied configs. That would be an erronious failure mechanism.

I think that the easiest solution here is to create a function returning a pair<> of FDBs (or perhaps better, a single use struct containing two FDBs) such that you can avoid this.

simondsmart · 2026-01-30T11:46:48Z

src/fdb5/tools/compare/fdb-compare.cc

+    }
+
+    if (refReqString_) {
+        opts_.referenceRequest = parseKeyValues(*refReqString_);


It isn't clear to me what a reference/test request is. And I am hardly a new user to this ecosystem. This needs some elaboration/commentary.

simondsmart · 2026-01-30T11:47:28Z

src/fdb5/tools/compare/fdb-compare.cc

+    }
+
+
+    // Return if only a comparison of Mars metadata messages was specified as Command Line option


Why does only this line warrant a comment. There are bits that could do with explanatory code. Not convinced this is the most significant...

simondsmart · 2026-01-30T11:48:59Z

src/fdb5/tools/compare/fdb-compare.cc

+    if (opts_.scope == Scope::All) {
+        std::cout << "****************** SUMMARY ********************" << std::endl;
+        std::cout << gribRes << std::endl;
+    }


I expect a compare tool to return success or failure depending on the output. This very much looks like it prints the output, but then returns zero. That makes it much harder to integrate into wider scripts.

simondsmart

A few extra details...

simondsmart · 2026-01-30T11:52:53Z

src/fdb5/tools/compare/common/DataMap.cc

+
+    for (const auto& [rk, rv] : r) {
+        auto search = l.find(rk);
+        if (search == r.end()) {


Bug here. search comes from l.find but is being compared against r.end. Iterators are not comparable between containers.

simondsmart · 2026-01-30T11:55:20Z

src/fdb5/tools/compare/fdb-compare.cc

+    }
+
+    if ((testReqString_ && !refReqString_) || (!testReqString_ && refReqString_)) {
+        throw UserError("Options --reference-request and --tests-request must either both be set or both be omitted.",


Should be --test-request not --tests-request

simondsmart · 2026-01-30T11:58:30Z

src/fdb5/tools/compare/grib/CompareBitwise.cc

+
+    // EditionNumber will is always the 8th Byte in a Grib message
+    int editionnumberRef  = (int)bufferRef[7];
+    int editionnumberTest = (int)bufferRef[7];


Both editionnumberRef and editionnumberTest are read from bufferRef, and so are equal by construction. Should be using bufferTest.

Please add a test to check that this is working.

simondsmart · 2026-01-30T12:00:42Z

src/fdb5/tools/compare/grib/CompareKeys.cc

+
+    if (holdsVector(valRef)) {
+        if (holdsVector(valTest)) {
+            auto l1 = vectorLength(valRef);


Note that vectorLength returns bool. So this is only testing if they are both empty/both have data. Doesn't test lengths.

oh. good catch.

stefaniereuter requested a review from Ozaq August 26, 2025 06:36

stefaniereuter marked this pull request as draft August 26, 2025 06:43

dsarmany force-pushed the feature/FDB-327-fdb-compare branch from 684c81d to 722f460 Compare October 21, 2025 09:34

pgeier force-pushed the feature/FDB-327-fdb-compare branch from 722f460 to 38d2604 Compare January 13, 2026 11:08

stefaniereuter force-pushed the feature/FDB-327-fdb-compare branch from 7b4aaf0 to 232d0b2 Compare January 15, 2026 12:38

pgeier force-pushed the feature/FDB-327-fdb-compare branch from d6efd5d to 957b3c0 Compare January 16, 2026 12:51

pgeier force-pushed the feature/FDB-327-fdb-compare branch 3 times, most recently from f6064e4 to c814917 Compare January 27, 2026 14:49

pgeier force-pushed the feature/FDB-327-fdb-compare branch from c814917 to 2daf3e2 Compare January 27, 2026 14:54

stefaniereuter added 18 commits January 28, 2026 14:54

added FDB compare to CmakeList tool is WIP and currently only loads t…

0cf5628

…wo FDB no comparison or testing done yet

Create FDB from path in tool.cc directly

f48b74f

comparison of mars keys

a7baa4d

can compare FDB but not optimized and exceptiions are missing

bbfaf0a

Bitexact Memcmp, bitexact eccodes, data compare numerical

4f8952f

move from map to vector in md5compare, add usage exampl and command l…

20fbf3c

…ine options

changed the way the mars message keys are compared,added option to ig…

d881e5b

…nore certain key-value pairs

Adding option to ignore and select grib keys

86f64ff

added gribcompare eccodes_detail to allow direct use of detailed check

d36ba97

removed annoying debug output

b8b8491

Added eccodes hack for buffer size adaption

8a63c45

remove eccodes 'hack' and use grib_string_length instead

7b7de8e

deleted buffer before return to avoid memleak

36f7f31

Bugfixes after rebase

81d0033

CLI improvments

80e58d7

Adding comparison tests

9eaf3aa

Feature, single fdb divergent experiment

9bafd3b

Add more tests, bugfix grib comparison header only

a1533d2

stefaniereuter and others added 14 commits January 28, 2026 14:54

Refactor Mars key compare

0bbd8ab

Mars refactor cleanup, Test fix

b15b50c

refactoring start

41ab0fb

formatting

74d19cb

Misc naminging and small structure fixes

c52b2bf

Rename and delete unused files

e42976e

Use metkit/codes/api instead of directly using eccodes\.h

9b6e1f6

Clean up compare/grib structure

b94f174

Rework handling difference in mars requests

9d3c367

Small restructures, remove comments

b59ff8e

Remove IComporator und move directories

eaeadf2

Restructure tests

78721dc

Fix fdb-compare tests and add FP data checks

09e5647

Add licensce to .h and .cc files

b643bc8

pgeier force-pushed the feature/FDB-327-fdb-compare branch from bcef5ed to b643bc8 Compare January 28, 2026 14:54

Add more documentation; remove grib/Message

1b031b4

pgeier force-pushed the feature/FDB-327-fdb-compare branch from 3508d48 to 1b031b4 Compare January 29, 2026 09:49

pgeier marked this pull request as ready for review January 29, 2026 10:02

simondsmart self-requested a review January 30, 2026 10:45

simondsmart requested changes Jan 30, 2026

View reviewed changes

		@@ -0,0 +1,7 @@
		ecbuild_configure_file( mismatch_mars.sh.in mismatch_mars.sh @ONLY )

		@@ -0,0 +1,7 @@
		ecbuild_configure_file( mismatch_grib.sh.in mismatch_grib.sh @ONLY )

		}


		// Return if only a comparison of Mars metadata messages was specified as Command Line option

Feature/fdb 327 fdb compare #165

Are you sure you want to change the base?

Feature/fdb 327 fdb compare #165

Conversation

stefaniereuter commented Aug 26, 2025 • edited by github-actions bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Contributor Declaration

Uh oh!

stefaniereuter commented Aug 26, 2025

Uh oh!

pgeier commented Jan 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Jan 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pgeier Jan 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

simondsmart left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

stefaniereuter commented Aug 26, 2025 •

edited by github-actions bot

Loading

pgeier commented Jan 27, 2026 •

edited

Loading

codecov-commenter commented Jan 28, 2026 •

edited

Loading

pgeier Jan 30, 2026 •

edited

Loading